-
Notifications
You must be signed in to change notification settings - Fork 418
#3618 followups + expose async receive feature #3999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#3618 followups + expose async receive feature #3999
Conversation
👋 I see @joostjager was un-assigned. |
Requested @TheBlueMatt since a lot of the code is things he requested in #3618 (review). |
b4a4826
to
1745ce0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3999 +/- ##
==========================================
- Coverage 88.93% 88.84% -0.09%
==========================================
Files 174 175 +1
Lines 124539 127634 +3095
Branches 124539 127634 +3095
==========================================
+ Hits 110758 113402 +2644
- Misses 11287 11678 +391
- Partials 2494 2554 +60
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
789b56b
to
7850181
Compare
🔔 1st Reminder Hey @TheBlueMatt @joostjager! This PR has been waiting for your review. |
1 similar comment
🔔 1st Reminder Hey @TheBlueMatt @joostjager! This PR has been waiting for your review. |
Now that we've implemented the static invoice server protocol, these methods are not intended to ever be called by users. If someone did want to use these apis, they could use the equivalent methods on the OffersMessageFlow.
We want the OffersMessageFlow to be usable by non-ChannelManager users, so expose the required methods to enable that. Also a few corresponding docs tweaks.
The previous API wouldn't work for language bindings.
The comment stated we need peers to be connected in order to send certain onion messages, but we actually need those peers available in order to create reply paths.
We were previously duplicating paths_to_static_invoice_server between the OffersMessageFlow and the AsyncReceiveOfferCache, even though the paths were only used in one place in the flow. De-duplicate that to remove the risk of the structs getting out-of-sync.
Makes the next commit a cleaner code move since the parameter will want to be removed then.
On startup, we want the async recipient to be set up with async receive offers as soon as possible. Therefore, attempt to send out initial offer_paths_requests as soon as the recipient is configured with blinded paths to connect to the static invoice server.
Previously, we were refreshing unused async receive offers based on when the corresponding invoice was confirmed as persisted. But the time when the invoice is persisted doesn't really tell us when the offer became stale, since messages may be delayed. Given we want to always have the freshest possible offer, we'd like to replace an unused offer with a new one every few hours based on the offer's age, regardless of when it was persisted by the server, which we now do.
Start tracking invoice creation time in cached async offers. This field will be used in the next commit to start only updating static invoices for Used offers every few hours instead of once a minute. We also remove the blinded path context StaticInvoicePersisted::path_absolute_expiry field here, replacing it with the new invoice_created_at field. We don't actually want to terminate early if the reply path a bit stale like we did before, since we want to use the invoice_created_at field regardless to drive a faster refresh of the invoice.
Previously, for every one of our async receive offers that are in use, we would send a fresh invoice once on every timer tick/once a minute. It'd be better to check if the invoice is actually kinda-stale before updating it to avoid bombarding the server, so we do so here using the new invoice_created field added in the previous commit.
Previously one of the static invoice server onion message TLV types conflicted with a DNS resolver onion message type, causing test failures on cargo test --cfg=async_payments. We also bump the TLV numbers by 10k until they can be documented in a bLIP.
We were previously cfg-gating all async payments code to not have a half-working public API, but now that async receive is usable go ahead and remove the gating. Paying as an async sender is not yet supported.
7850181
to
c0b8a44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clean!
Completes the follow-ups from #3618 (review) and exposes the async payments feature for usage as a recipient. Paying as an async sender is not yet supported.